Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow global resolver Middlewares #1127

Merged
merged 7 commits into from
Mar 4, 2024

Conversation

alancolant
Copy link
Contributor

@alancolant alancolant commented Feb 27, 2024

Summary

Add possibility to define Global Resovler Middleware using GraphQL Facade.
Usefell when external service like "Laravel Pulse" recorder want to register globally new middleware to intercept each query and track execution time.

This can be done by using the new GraphQL::appendGlobalResolverMiddleware(YourMiddleware::class) method

You can see in this repo why I need: https://github.com/alancolant/laravel-pulse-graphql


Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Misc. change (internal, infrastructure, maintenance, etc.)

Checklist:

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md
  • Code style has been fixed via composer fix-style

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach LGTM!

Can you please:

  • add tests
  • add a changelog entry
  • update readme
    ?

@alancolant
Copy link
Contributor Author

alancolant commented Feb 27, 2024

Ok, tests added, and changelog.md tagged as 9.4.0

@illambo
Copy link
Contributor

illambo commented Feb 27, 2024

Hi, interesting! The only doubt I have concerns whether there is a problem regarding data security, in the sense that by doing so you offer an api that any vendor could use and intercept / listen to past data (maybe manipulate them?) without you realizing it. Is what I say correct or I miss something (maybe paranoia 😅)? Anyway thanks for the PR.

@alancolant
Copy link
Contributor Author

Yes, that’s right, that’s why I need it 😅
If the goal is to protect the library from external vendors, this vendor would still have the possibility of injecting itself by extending the configuration of execution middleware before the app container resolves GraphQL, which generates the same risks.

It could also be placed at the top level (Http Kernel Middleware), in which case the library itself would not be able to ensure "its own security"

@illambo
Copy link
Contributor

illambo commented Feb 28, 2024

Yes, I don't think we can act to protect in some way, it was just a thought on the matter that I wanted to share.

@alancolant
Copy link
Contributor Author

@mfn Can you merge please?

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've a feeling there's something missing from the PR.

A LOT of behaviour is controlled via the config file, you can define all the schemas, middlewares (resolver middlewares) and execution middlewares there.

In fact, you can already define execution middlewares per schema and global one. However, the per-schema execution middlewares completely override the global one.

Therefore I think, adding the ability to append global resolver middlewares but not exposing them in the config (resolver_middleware_append?) seems imbalanced.

WDYT?

@alancolant
Copy link
Contributor Author

I've a feeling there's something missing from the PR.

A LOT of behaviour is controlled via the config file, you can define all the schemas, middlewares (resolver middlewares) and execution middlewares there.

In fact, you can already define execution middlewares per schema and global one. However, the per-schema execution middlewares completely override the global one.

Therefore I think, adding the ability to append global resolver middlewares but not exposing them in the config (resolver_middleware_append?) seems imbalanced.

WDYT?

It's a good idea, I push new changes with possibility to use resolver_middleware_append key in config file too.

As the middleware receives the schema information as the first parameter, I do not think that it is useful to complicate the code base with management by schema, it will be enough for the middleware to do nothing if the schemaName is not suitable.
This will also allow the middleware to apply their own logic based on the schema

@mfn mfn self-assigned this Mar 4, 2024
Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid, thank you!

@mfn mfn merged commit 91735a9 into rebing:master Mar 4, 2024
17 checks passed
@mfn
Copy link
Collaborator

mfn commented Mar 4, 2024

Released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants